Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Oauth2 feature added #67

Open
wants to merge 50 commits into
base: main
Choose a base branch
from
Open

Oauth2 feature added #67

wants to merge 50 commits into from

Conversation

fepatrifork
Copy link

@fepatrifork fepatrifork commented May 8, 2024 •
edited
Loading

Description

These changes add the capability to manage the Oauth2 authentication/authorization using JWT token provided by an IdP. The modification involves different levels of the system that is possible to refer as frontend for the part of the powerBI connector that is defined in Power Query (M) and backend for the part that contains the ODBC driver logic. The modification on the frontend (PowerBI connector) comprehends the management of the Authorization Code Flow "OAUTH2" using the predefined callbacks StartLogin, FinishLogin, Refresh, Logout and the configuration of the flow comes from a JSON file (template provided). This level passes the token to the backend (ODBC driver) that manages the information received to make an HTTP request with a different header. The modification includes the" ODBC Data Sources Administrator" that now allows the selection of the "OAUTH2" option for inserting a token in a created field and testing the connection. The modification of the token information has a similar treatment of the password in "BASIC" during the whole logic, with adapting due to the string size of the token. After installing the re-packaged .msi file, a system test was settled and passed using "PowerBI Desktop" and in " ODBC Data Sources Administrator" with a localhost database. The result of unit-test integration-test are made with the provided script test_runner.py modified to exclude .recipe extension that does not allow running all the tests. Test cases, documentation, user manual and DCO can be provided and expanded if there is an interest in this work and you consider it valuable.

Issues Resolved

Added Oauth2 feature

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
    • Not all tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@fepatrifork fepatrifork mentioned this pull request May 8, 2024
Open
rupal-bq
rupal-bq reviewed May 21, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
bi-connectors/PowerBIConnector/src/OpenSearchProject.pq Outdated Show resolved Hide resolved
bi-connectors/PowerBIConnector/src/OpenSearchProject.pq Outdated Show resolved Hide resolved
@rupal-bq
Copy link

Can you please take a look at failed CI?

@fepatrifork
Copy link
Author

fepatrifork commented May 21, 2024 •
edited
Loading

Can you please take a look at failed CI?

I take care of the followings:

  • I have to add the DCO for all commits
  • Some weblinks are missing endpoint in old .md files (not modified from this pull request)
    - SOLUTION: modify the missing endpoint with the redirected link or refer a MD file from this repo
  • Linux environment seems having problem in CPPCheck
    - ERROR MESSAGE: "src/opensearchenlist/msdtc_enlist.cpp:1114:49: error: There is an unknown macro here somewhere.
    Configuration is required. If KEYWORD_DTC_CHECK is a macro then please configure it. [unknownMacro] strcat(dtcname,
    ";" KEYWORD_DTC_CHECK "=0");".
    - SOLUTION: pushed a solution for msdtc_enlist.cpp.

Looking forward for feedback after re-CI!

gaiksaya and others added 8 commits May 21, 2024 22:33
@gaiksaya @fepatrifork
Fix source path for jenkins file (opensearch-project#59)
b5130a1 
Signed-off-by: Sayali Gaikawad <[email protected]>
Signed-off-by: fepatrifork <[email protected]>
@gaiksaya @fepatrifork
Override default release docker image (opensearch-project#60)
ab6870e 
Signed-off-by: Sayali Gaikawad <[email protected]>
Signed-off-by: fepatrifork <[email protected]>
@gaiksaya @fepatrifork
Update jenkins file to use updated docker image (opensearch-project#61)
cc17bee 
Signed-off-by: Sayali Gaikawad <[email protected]>
Signed-off-by: fepatrifork <[email protected]>
@fepatrifork
Create Trifork.readme.md
6e31762 
Signed-off-by: fepatrifork <[email protected]>
Signed-off-by: fepatrifork <[email protected]>
@fepatrifork
Beta version using password field as JWT entry point, tested
c7660bf 
Signed-off-by: fepatrifork <[email protected]>
@fepatrifork
Beta version using password field as JWT entry point, tested
1aede8b 
Signed-off-by: fepatrifork <[email protected]>
@fepatrifork
Beta version using only JWT as entry point, readme inserted, tested
2e4990c 
Signed-off-by: fepatrifork <[email protected]>
@fepatrifork
Update README.md
3caa072 
Signed-off-by: fepatrifork <[email protected]>
Signed-off-by: fepatrifork <[email protected]>
fepatrifork and others added 12 commits May 21, 2024 22:33
@fepatrifork
Connector modified, tested, CICD docker adapted
9886b55 
Signed-off-by: fepatrifork <[email protected]>
@fepatrifork
Prepare the branch for merge the fork
ca34449 
Signed-off-by: fepatrifork <[email protected]>
@fepatrifork
Prepare the branch for merge the fork
aa7ab73 
Signed-off-by: fepatrifork <[email protected]>
@fepatrifork
Prepare the branch for merge the fork
53aeaba 
Signed-off-by: fepatrifork <[email protected]>
@fepatrifork
Remove internal file
1eeb667 
Signed-off-by: fepatrifork <[email protected]>
@fepatrifork
Prepare the branch for merge the fork
1ce0168 
Signed-off-by: fepatrifork <[email protected]>
@fepatrifork
PowerBi connector now use config file for set up Oauth flow
0cff5c3 
Signed-off-by: fepatrifork <[email protected]>
@fepatrifork
PowerBi connector now use Json config file for set up Oauth flow
3772e9f 
Signed-off-by: fepatrifork <[email protected]>
@fepatrifork
Manual updated
661f2e3 
Signed-off-by: fepatrifork <[email protected]>
@fepatrifork
Update OpenSearchProject.md
09a3bd4 
Update readme OpenSearchProject.md

Signed-off-by: fepatrifork <[email protected]>
Signed-off-by: fepatrifork <[email protected]>
@fepatrifork
Update .gitignore
073fc2b 
Signed-off-by: fepatrifork <[email protected]>
@fepatrifork
Update .pq
aa81eac 
Signed-off-by: fepatrifork <[email protected]>
@rupal-bq
Copy link

@fepatrifork Thanks for looking into CI failures. Can you please also add test cases, documentation and user manual?

@fepatrifork
Copy link
Author

fepatrifork commented May 22, 2024 •
edited
Loading

@fepatrifork Thanks for looking into CI failures. Can you please also add test cases, documentation and user manual?

You're welcome. I see some other error:

  1. Link Checker now should finally ok with the last push
  2. Can you provide help with the error of cppcheck? Before was a clear error, now it seems all ok, but do not pass the test.

For the next steps:

  1. For documentation and user manual I can integrate and enrich the pre-existent .md file for PowerBI connector and for the driver. Is that ok?
  2. For the test cases will take some time.
    • SOLUTION: Added tests in test_conn.cpp in UnitTests and an IntegrationTest in ITODBCOauth folder. Integration test use
      a function that make a request to the IdP with client-credentials flow to get the token. Of course is fitted for our system, if
      you have an active webAPI with an IdP that can be reached with a test user, it will be perfect.

@rupal-bq
Copy link

Can you provide help with the error of cppcheck? Before was a clear error, now it seems all ok, but do not pass the test:

Sure, I'll take a look

For documentation and user manual I can integrate and enrich the pre-existent .md file for PowerBI connector and for the driver. Is that ok?

Yes

@fepatrifork
Copy link
Author

fepatrifork commented May 28, 2024 •
edited
Loading

  • Some tests did not pass, because of some number of values returned are different, for example in test_query_execution.cpp the test:

all_columns_flights_count = 25 is not equal to
result -> num_fields = 27

  • I take a look over the Cppcheck error running, it seems that the only error resides inside in the vcpkg src\vcpkg_installed\..... that belongs to external packages manager I suppose. The others are only information-messages, that I hope they are not considered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rupal-bq rupal-bq rupal-bq left review comments

@ps48 ps48 Awaiting requested review from ps48 ps48 is a code owner

@kavithacm kavithacm Awaiting requested review from kavithacm kavithacm is a code owner

@derek-ho derek-ho Awaiting requested review from derek-ho derek-ho is a code owner

@joshuali925 joshuali925 Awaiting requested review from joshuali925 joshuali925 is a code owner

@dai-chen dai-chen Awaiting requested review from dai-chen dai-chen is a code owner

@YANG-DB YANG-DB Awaiting requested review from YANG-DB YANG-DB is a code owner

@mengweieric mengweieric Awaiting requested review from mengweieric mengweieric is a code owner

@vmmusings vmmusings Awaiting requested review from vmmusings

@Swiddis Swiddis Awaiting requested review from Swiddis Swiddis is a code owner

@penghuo penghuo Awaiting requested review from penghuo penghuo is a code owner

@seankao-az seankao-az Awaiting requested review from seankao-az seankao-az is a code owner

@MaxKsyunz MaxKsyunz Awaiting requested review from MaxKsyunz MaxKsyunz is a code owner

@Yury-Fridlyand Yury-Fridlyand Awaiting requested review from Yury-Fridlyand Yury-Fridlyand is a code owner

@anirudha anirudha Awaiting requested review from anirudha anirudha is a code owner

@forestmvey forestmvey Awaiting requested review from forestmvey forestmvey is a code owner

@acarbonetto acarbonetto Awaiting requested review from acarbonetto acarbonetto is a code owner

@GumpacG GumpacG Awaiting requested review from GumpacG GumpacG is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fepatrifork @rupal-bq @gaiksaya